fix: preserve vertex attributes with smooth normals - #9233

Continuous Release

CDN link

• https://raw.esm.sh/pr/p5@071aeca/lib/p5.min.js

Published Packages

• https://pkg.pr.new/p5@071aeca

Commit hash: 071aeca

This is an automated message.

Logic looks good! To confirm that the normals themselves haven't changed since before, would you be up for adding a visual test with maybe normalMaterial() on a shape with computeNormals(SMOOTH) applied so that we can more easily visually inspect it? If you need some example geometry, there's an example sketch in the "smooth shading" section of https://p5js.org/tutorials/custom-geometry/ that we could borrow.

Logic looks good! To confirm that the normals themselves haven't changed since before, would you be up for adding a visual test with maybe normalMaterial() on a shape with computeNormals(SMOOTH) applied so that we can more easily visually inspect it? If you need some example geometry, there's an example sketch in the "smooth shading" section of https://p5js.org/tutorials/custom-geometry/ that we could borrow.

sure

Logic looks good! To confirm that the normals themselves haven't changed since before, would you be up for adding a visual test with maybe normalMaterial() on a shape with computeNormals(SMOOTH) applied so that we can more easily visually inspect it? If you need some example geometry, there's an example sketch in the "smooth shading" section of https://p5js.org/tutorials/custom-geometry/ that we could borrow.

hey @davepagurek added the requested visual test for computeNormals(SMOOTH) using normalMaterial() on the warped tube geometry from the smooth shading example.
The visual test passes and the expected screenshot has been generated pls check it and let me know if any issues

Hi @Srinidhi444! When you add visual tests, you have to run them locally and commit the image files that get added to the project. The idea is that those images remain checked into source control so that when future changes happen, the tests are rerun and compared to those images to make sure that nothing breaks.

Hi @Srinidhi444! When you add visual tests, you have to run them locally and commit the image files that get added to the project. The idea is that those images remain checked into source control so that when future changes happen, the tests are rerun and compared to those images to make sure that nothing breaks.

hey @davepagurek my bad didn't knew that i have committed the necessary changes let me know if they are correct.

Hmm when I run the tests on CI, it says it expected this:

but received this:

Any idea where the difference comes from? Do you get that when running it locally?

Hmm when I run the tests on CI, it says it expected this:

but received this:

Any idea where the difference comes from? Do you get that when running it locally?
Hey @davepagurek, I found the issue. It was caused by p5.noise() generating a different noise pattern between test runs, which resulted in different radii and therefore different screenshots. I fixed it by setting a fixed noise seed with p5.noiseSeed(0).

@all-contributors please add @Srinidhi444 for code

@davepagurek

I've put up a pull request to add @Srinidhi444! 🎉

@Srinidhi444 are you interested in making follow-up issues for the next steps mentioned in #9205 ? Feel free to do so + also work on them if you're up for it!

@Srinidhi444 are you interested in making follow-up issues for the next steps mentioned in #9205 ? Feel free to do so + also work on them if you're up for it!

@davepagurek yes sure i will understand them first properly and then will come with a approach and then will let you know

Hey @davepagurek!

I wanted to confirm my understanding of the remaining improvements mentioned in #9205 before creating the follow-up issues.

From what I understand, there are two main changes left:

1. Rename FLAT to KEEP_CONNECTIONS

The first change would be to introduce KEEP_CONNECTIONS for the existing FLAT behavior, while keeping FLAT as a deprecated alias for backward compatibility.

Just wanted to clarify one thing here: is the plan to keep FLAT as a deprecated alias indefinitely, or would we eventually remove it in a future version? Also, should we mark it as deprecated in the documentation for now?

2. Introduce SEGMENTED for true flat shading

The second change would be to implement actual flat shading, which, from my understanding, is somewhat the opposite of what we did for SMOOTH.

My current understanding of the approach is:

• Calculate the normal of each face separately.
• Create independent vertices for each triangle so that shared vertices can have different normals.
• Duplicate the corresponding UVs and other per-vertex attributes to preserve their alignment.
• Assign each triangle's face normal to its own vertices.
• Update the face indices to reference the new vertices.

Something like this, if I'm understanding it correctly:

3. Selective smoothing / sharp edges

I also saw your suggestion about allowing some edges to remain sharp while others are smoothed, possibly through an additional options object.

I haven't explored this part deeply yet, as I think it would require some more thought and investigation. For now, I was planning to focus on the first two improvements and revisit this later.

Regarding the follow-up issues and PRs:

I was thinking of creating two separate issues and two separate PRs:

• One for introducing KEEP_CONNECTIONS and deprecating FLAT.
• Another for implementing SEGMENTED with true flat shading.
  i think that would be clean ,let me know if i have misunderstood something or there is a better approach before creating the issues.

Thanks!